Custom colour themes - #47

Hope you'll forgive the repeated squashing and amending - tried to keep history clean but I found out too late that there was a cheap way of doing basic colour compatibility checking. Raised #48 to track the more robust way of doing it.

Found a bug in the background alias before merging.

The issue: In apply_custom_fields (tui.rs), the background alias sets current_row_bg and current_cell_bg to the same value. A user who writes:

[[theme.custom]]
name = "tokyonight"
background = "#1a1b26"

loses row highlighting entirely — every row has the same background color, making navigation harder in a spreadsheet viewer.

Confirmed with a test:

#[test]
fn test_background_alias_row_highlight_distinct() {
    let custom = crate::config::CustomTheme {
        name: "NavTest".into(),
        inherits: Some("Default".into()),
        background: Some(Color::Rgb(26, 27, 38)),
        ..Default::default()
    };
    let themes = resolve_themes(&[custom]).unwrap();
    let t = &themes[6];
    assert_ne!(t.colors.current_row_bg, t.colors.current_cell_bg);
}

Output:

assertion `left != right` failed: background alias collapses current_row_bg and current_cell_bg to the same value, making row highlighting invisible
  left: Rgb(26, 27, 38)
 right: Rgb(26, 27, 38)

Fix: remove current_row_bg from the background alias block. It'll inherit from the parent theme (which already has a contrasting value), and users who want to override it can set current_row_bg explicitly.

Everything else looks great — happy to approve once this is addressed.

Argh! Completely missed that. Worse - it actually extends further than just that one case. It looks like foreground and background aliases were both affected.

I decided that aliases should only cover elements that are meant to look uniform. Elements whose purpose are to provide contrast should inherit from parent even when foreground and background aliases have been added. This means that elements that are intended to look different will inherit contrasting colours from the parent where they were designed in.

If users want to override that, they still can, they just have to explicitly set the additional values.

I've also generalised the test you added to catch the broader class of issues and have refactored parts of the code that failed that test.

Hey @AlexanderNZ, heads up: I just merged #49, which was a big refactor. src/tui.rs is now a tui/ module with the theme code living in src/tui/theme.rs, so this branch won't merge cleanly anymore. Sorry for the churn!

The good news is the new structure should make this feature easier to land, since themes now have their own module instead of being buried in a 2k line file. If you're still up for it, could you rebase onto main and move the custom theme logic into src/tui/theme.rs? Happy to answer questions about the new layout. If you don't have the time, no worries, just let me know and I'll keep #42 open as the tracking issue for someone to pick up.

Nice refactor. Makes this work a bit cleaner.

I've made four separate commits where it probably could have been one or two, I wanted to break out the refactor of your code from the feature changes so you can drop anything that doesn't sit right without unpicking the rest.

The true colour warning now only nags on custom themes. I didn't want to spam people who hadn't actually touched any of their colour config.

I've added a sixth commit that collapses CustomTheme / apply_custom_fields / uses_rgb into a single macro_rules! table. After my additions those three lists need to stay in sync across two files and it felt like a maintenance trap waiting to happen. But you just refactored that code, so I've broken it out as the last commit in case you'd prefer to drop it.

Hey @AlexanderNZ, thanks for the rebase and for splitting the commits the way
you did. Being able to read the macro change on its own made this much easier
to review. Sorry it took a while to get back to you.

Heads up on one thing I changed: I retargeted this PR to devel. That's the
default/integration branch now, and main just tracks releases. Nothing for
you to do, the diff was unaffected.

I went through the whole thing. Short version: this is good, and I want to
land it.

The alias fix. You went further than the bug I reported, and I checked
that it holds by construction rather than just by the test passing. Every
field whose job is contrast (current_cell_fg/bg, current_row_bg,
current_col_fg, and the four search colors) is tagged none in the table,
so an alias physically can't reach them. The generalized test is a superset
of my original assertion. Nice.

inherits. I poked at the cases you didn't call out: forward references,
self-inheritance, and a custom named after a built-in inheriting itself. They
all behave the way I'd want, and rejecting forward references so cycles can't
be expressed is the right trade. Good call not writing a cycle detector.

The macro commit: keeping it. I know you offered to drop it since I'd just
touched that code. It removes about 46 lines net and folds three lists that
had to stay in sync across two files into one table. That sync problem is what
produced the alias bug in the first place, so this is the change that stops it
recurring. Worth the small hit to grep-ability.

deny_unknown_fields: keeping that too. It only covers [[theme.custom]]
rather than the whole config, and with 20 similarly-named color fields the typo
protection is worth more than the forward-compat cost.

One change before I merge:

truecolor_warning is called in main.rs before the if cli.interactive
branch, so xleak book.xlsx --export csv > out.csv can emit a terminal-color
warning on stderr for output that has no colors in it. Could you gate that one
call on cli.interactive? To be clear, I only mean the warning. Resolving
themes eagerly so a bad inherits fails in both modes is right, keep that.

And one wording nit, no code change needed. The warning returns early unless
the active theme is custom, but as your own all_builtins_use_rgb test shows,
every built-in uses RGB. So someone on Terminal.app running built-in Nord, who
is basically the person #48 is about, won't see it. I think the anti-nag
scoping is the right behavior, but the CHANGELOG entry crediting #48 promises
more than it delivers. Mind softening that line to describe just the custom
theme case?

Test coverage is solid, and I appreciate that the tests look themes up by name
instead of hardcoding cycle positions.

Once those two are in I'll merge. You shouldn't need another rebase, #69
touches CHANGELOG.md and README.md too but I'm landing yours first, so that
conflict is theirs to resolve, not yours.

Let me know if you run into issues!

Sweet! Made those two changes just now :)